Skip to content
This repository has been archived by the owner on Jul 30, 2018. It is now read-only.

WebAnimations Meta #713

Merged
merged 33 commits into from
Nov 7, 2017
Merged

WebAnimations Meta #713

merged 33 commits into from
Nov 7, 2017

Conversation

tomdye
Copy link
Member

@tomdye tomdye commented Oct 9, 2017 •
edited
Loading

Type: feature

The following has been addressed in the PR:

  • There is a related issue
  • All code matches the style guide
  • Unit or Functional tests are included in the PR

Description:
Adds a WebAnimations meta which allows programatic web animations to be applied to a VNode.

import WebAnimation from '@dojo/widget-core/meta/WebAnimation';

export default class AnimatedWidget extends WidgetBase {
    protected render() {
        const animate = {
            id: 'rootAnimation',
            effects: [
                { height: '10px' },
                { height: '100px' }
            ],
            controls: {
                play: true
            }
        };

        this.meta(WebAnimation).animate('root', animate);

        return v('div', {
            key: 'root'
        });
    }
}

Resolves #657

@kitsonk kitsonk requested review from agubler and matt-gadd October 9, 2017 15:33
@kitsonk kitsonk added the beta4 label Oct 9, 2017
@dylans dylans added this to the 2017.10 milestone Oct 10, 2017
@kitsonk kitsonk modified the milestones: 2017.10, beta.4 Oct 30, 2017
@tomdye tomdye force-pushed the animations branch 3 times, most recently from afcaa60 to 59867fb Compare October 31, 2017 16:46
@tomdye tomdye changed the title AnimatableMixin AnimatedMixin Oct 31, 2017
agubler
agubler reviewed Oct 31, 2017
View reviewed changes
Copy link
Member

@agubler agubler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple of very minor nits and questions

README.md Outdated
const { scroll } = this.meta(Dimensions).get('content');

return [
{ height: `0px` },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we don't need ` here, just '

src/mixins/Animated.ts Outdated
import Map from '@dojo/shim/Map';
import MetaBase from '../meta/Base';

declare const KeyframeEffect: any;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not type these?

src/mixins/Animated.ts Outdated
(node: HNode) => {
const { animate, key } = node.properties;
if (animate && key) {
this.meta(AnimationPlayer).add(key as string, animate, this);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason we can't bind the controls in here rather than in the meta?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can but it's potentially an array of arrays so wanted to avoid the extra loops. Can do though if you feel it's better?

src/mixins/Animated.ts Outdated
decorate(result,
(node: HNode) => {
const { animate, key } = node.properties;
if (animate && key) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should warn if animate is passed without a key, might be surprising if it doesn't work?

@tomdye tomdye force-pushed the animations branch 2 times, most recently from 8ffa0b6 to 26ecd13 Compare November 2, 2017 14:16
@tomdye tomdye changed the title AnimatedMixin WebAnimations Meta Nov 2, 2017
@tomdye
Copy link
Member Author

tomdye commented Nov 3, 2017

@agubler ready for another review

agubler
agubler reviewed Nov 6, 2017
View reviewed changes
src/interfaces.d.ts Outdated
@@ -414,6 +455,7 @@ export interface NodeHandlerInterface extends Evented {
export interface WidgetMetaProperties {
invalidate: () => void;
nodeHandler: NodeHandlerInterface;
bind: any;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really any?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the bind scope is any in widgetBase I believe? I guess it should probably be WidgetBaseConstructor ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wouldn't be the constructor, it would just be WidgetBase if it is able to be typed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

agubler
agubler reviewed Nov 6, 2017
View reviewed changes
src/meta/WebAnimation.ts Outdated
}

afterRender() {
super.afterRender();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why super.afterRender()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this not how you call the super function?

@agubler
Copy link
Member

agubler commented Nov 6, 2017

package-lock.json conflict now

@tomdye
Copy link
Member Author

tomdye commented Nov 7, 2017

@agubler all done, please re-review

@tomdye tomdye merged commit 7267224 into dojo:master Nov 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@agubler agubler agubler approved these changes

@matt-gadd matt-gadd Awaiting requested review from matt-gadd

Assignees
No one assigned
Labels
beta4
Projects
None yet
Milestone
beta.4
Development

Successfully merging this pull request may close these issues.
4 participants
@tomdye @agubler @dylans @kitsonk